Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merges with options earlier in sequence and handles #164 #175

Closed
wants to merge 1 commit into from

Conversation

hazah
Copy link

@hazah hazah commented Apr 5, 2017

No description provided.

@hazah
Copy link
Author

hazah commented Nov 20, 2017

Can this be merged into the main project?

@mattpolito
Copy link
Member

@hazah Thanks for the PR.

We've honestly never found a reason to use DecentExposure in the way that #164 describes which is why I'm hesitant to make this change. Could you add another example of what this is solving and a test case to show that it's actually doing what you expect?

@hazah
Copy link
Author

hazah commented Nov 20, 2017

The number one thing this would resolve is consistency. It is currently possible to use the from option with the decorate option (https://github.com/hazah/decent_exposure/blob/master/spec/decent_exposure/controller_spec.rb#L375). As far as I can tell, the with option is there to have the ability to group together various configurations, and so it should be seamless, as if it was never even used as an option, when actually used.

I will have to conjure up another example, but authorisation is the one that comes up consistently in every single project. At the moment though, I cannot see a case where it makes sense to have a named configuration that cannot be used simply due to the fact that another option happened to be active. I can understand if the with option has, within it, an incompatible option instead. That would make sense to blow out of the water.

I am basically arguing that once options have been pre-processed, there should be no such thing as a with option.

@mattpolito mattpolito deleted the branch hashrocket:master December 6, 2022 02:18
@mattpolito mattpolito closed this Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants